Skip to content

Add VariantArray duplicate method for deep copy #711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 15, 2021
Merged

Add VariantArray duplicate method for deep copy #711

merged 1 commit into from
Mar 15, 2021

Conversation

mivort
Copy link
Contributor

@mivort mivort commented Mar 15, 2021

According to Godot's Array docs, duplicate method can accept deep boolean parameter to perform a deep array copy. This patch adds this functionality to VariantArray's interface. Simple deep copy functionality test was also added to test_array section.

@mivort
Add VariantArray duplicate method for deep copy
973229f 
Added `duplicate_deep` method for VariantArray using existing deep
option from Godot. Testcase for deep copy was also added to `test_array`
section.
ghost
ghost approved these changes Mar 15, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

bors r+

@ghost ghost added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Mar 15, 2021
@bors
Copy link
Contributor

bors bot commented Mar 15, 2021

Build succeeded:

@bors bors bot merged commit 7de34a0 into godot-rust:master Mar 15, 2021
@Bromeon
Copy link
Member

Bromeon commented Mar 15, 2021

Quick question regarding method names, we have

  • Variant::clone()
  • GodotString::clone()
  • Dictionary::duplicate()
  • VariantArray::duplicate()
  • VariantArray::duplicate_deep()

And for ref-increment we use new_ref(), if I'm not mistaken.

From a user perspective, maybe we should highlight this somewhere in the documentation?

Is there a semantic difference between clone and duplicate[_deep] or is the idea "when it supports both shallow and deep copy, use duplicate/duplicate_deep, otherwise use clone?

@ghost
Copy link

ghost commented Mar 17, 2021

@Bromeon I'm aware that it's a bit messy right now. For clone vs duplicate, my understanding is that Clone on a reference-counted type should be a reference increment (i.e. not even a shallow copy), to be consistent with standard types like Rc, Arc, even though the reference-counting is "internal" in the case of variant collections. That does make NewRef seem redundant, and I think that would warrant a separate issue for discussion.

@Bromeon Bromeon mentioned this pull request Mar 17, 2021
Open
@ghost ghost mentioned this pull request May 1, 2021
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer
Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mivort @Bromeon